Add virtual destructor #12
Conversation
The Bmi base class definition needs to define its destructor as `virtual` to enable derived classes to support accurate destruction. Cf https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rh-dtor This is technically an ABI change impacting any existing code. To avoid potential failures and definite UB from ODR violation, all clients and implementations within a given system will need to be (re-) built against matching versions of this header. It's unclear whether it's a breaking change of the BMI specification itself. The depends on whether the reference to this file is normative or exemplary.
|
The ABI change is probably not too consequential for most users. I'm roughly guessing that the sprawling set of components and large multi-team project of the NWM is going to be the worst impacted. It's also where I saw this come up, due to GCC's |
|
@mdpiper What's the process look like for actually merging a change like this? |
|
@PhilMiller I'd like someone with C++ knowledge to review this. Would you be able to recommend someone? After review, we should be able to merge this with lazy consensus. |
|
@hellkite500 knows the relevant considerations on both the C++ and BMI sides well enough. We'd already discussed the patch before I posted it. Otherwise, I could solicit someone working on the Kokkos Tools, I guess, as folks with ABI evolution constraints on a plugin system design. From a strictly C++ development perspective, I think of this as an utterly mundane, uncontroversial change. The challenge as I see it is entirely from the BMI ecosystem consideration. In the long run, it should fix more bugs than it creates, but it has potential for extended 'short-term' pain as people get the message that they need to transition by rebuilding all their code with the revised header. |
There was a problem hiding this comment.
This change is needed for proper destruction of subclasses of Bmi using dynamic dispatch to virtual functions through a Bmi base pointer. This is common practice in interface design/implementation using C++ .
In practice, I don't think too many implementations of C++ bmi models are going to be using custom destructors, so this probably hasn't been a problem. Typically "destruction" semantics are likely done in the Finalize function and implementers likely don't define custom destruction beyond that in many cases.
However, there may be use cases in which custom destruction semantics are used/needed, and without the virtual destructor, calls to a Bmi base pointer's destructor will only call the (default, do nothing) Bmi::~() and the subclass destructor won't get dispatched to at runtime.
Adding this does, as noted, introduce an ABI change for the C++ implementation only. There doesn't seem to be real consensus yet (see this disscussion thread) as to what level of ABI compatibility BMI should strive to maintain, both at the conceptual interface layer of BMI and at the individual implementations such as bmi-cxx. That said, I think a release tag documenting the change and the possible implications and fix (recompile components with new header) would likely be sufficient to communicate to users/developers.
@RolfHut tagging you here as you have expressed interest in the ABI compatibility, and may want to track this merge and re-build if you have C++ components in any workflows.
|
@PhilMiller I've tagged this |
|
Can we get someone that worked on GRPC4BMI to also review this? As a PI I can not oversee the impact this has, it feels very specific to C++ and not to BMI in general, but this is beyond my knowlegde. Given that in GRPC4BMI we also work with C++, it might impact it. Maybe @nielsdrost can suggest who would be best for this? |
|
@RolfHut @nielsdrost Would you still like someone from @eWaterCycle to look at this PR? If not, let me know & I'll merge it. |
|
yes I would, but personally I can not oversee the scope of this. Maybe @BSchilperoort or @Peter9192, but since they are both not in this org, I will email them. Please give a few days before laze merging. |
|
@RolfHut Since this is such a touchy change, would it be possible to get comments affirming that they think this is OK? As eager as I am to see it move forward, the sort of breakage this can cause if people are caught unaware is really painful to debug, so I'd like to avoid that. |
We haven't had time to look into this in detail, but a dedicated release tag would be great so we can always pin to v2.0 as a (temporary) workaround for unforeseen issues in the future. |
|
@mdpiper see @Peter9192 his comment, if you create that dedicated release tag, than all ok with us from eWaterCycle. |
In grpc4bmi we don't subclass the BMI C++ interface but we wrap it. This means the missing virtual keyword in the destructor doesn't affect this code and rebuilding e.g. containers with this correct destructor in place is trivial. I'm not sure whether we have C++ models in eWaterCycle that do any cleanup in the destructor (usually 'finalize' is used for that), if they did they would have suffered from a memory leak. Which is then fixed with this change. |
|
Ok, it sounds like we've got good awareness among folks who are most likely to be affected, general support, and no objections. I think we're clear to merge, since I was the one asking us to be more stringent about affirmative responses to this change as opposed to lazy consensus. |
|
@Peter9192 @RolfHut It looks like there are already tags in the repository Since I believe the major version is meant to be kept aligned with the BMI standard itself, I'd suggest the tag with this change merged be name |
|
Alternatively, if this is considered a bugfix then semantic versioning would specify to increase the patch number, i.e. v2.0.3. I don't have a strong opinion on this though, as long as the changes associated with each version are clearly described. |
|
Strict application of SemVer would call this 3.0, due to the lack of compatibility. SemVer doesn't really address ABI issues separate from API, though. It also doesn't address alignment of a product with a standard specification |
|
@mdpiper Would you like to press the Merge button? I'm feeling a bit too timid to do that myself as my first applied change here. |
|
@PhilMiller Let’s also start a discussion on the BMI forum about versioning. @mcflugen & I threw around some ideas a few years ago, but we weren’t happy with any of them. It might be nice to work on this again with more people involved. |
The Bmi base class definition needs to define its destructor as
virtualto enable derived classes to support accurate destruction.Cf https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rh-dtor
This is technically an ABI change impacting any existing code. To avoid potential failures and definite UB from ODR violation, all clients and implementations within a given system will need to be (re-) built against matching versions of this header.
It's unclear whether it's a breaking change of the BMI specification itself. The depends on whether the reference to this file is normative or exemplary.
Resolves #11